feat: Expanded mode for global drawers #3381
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3381 +/- ##
========================================
Coverage 96.54% 96.55%
========================================
Files 807 807
Lines 23390 23450 +60
Branches 7708 8172 +464
========================================
+ Hits 22581 22641 +60
+ Misses 802 755 -47
- Partials 7 54 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
georgylobko
changed the title
georgylobko
requested review from
pan-kot and
just-boris
and removed request for
a team
src/app-layout/interfaces.ts
Outdated
| @@ -318,6 +318,7 @@ export namespace AppLayoutProps { | |||
| triggerButton?: string; | |||
| resizeHandle?: string; | |||
| resizeHandleTooltipText?: string; | |||
| expandedModeButton?: string; | |||
There was a problem hiding this comment.
⛳ How about splitting this interface in two separate for local and global drawer labels?
Also, global drawers cannot be set using React API, so we should not expose this interface here
| @@ -64,10 +68,14 @@ function AppLayoutGlobalDrawerImplementation({ | |||
| const size = getLimitedValue(minDrawerSize, activeDrawerSize, maxDrawerSize); | |||
| const lastOpenedDrawerId = drawersOpenQueue.length ? drawersOpenQueue[0] : null; | |||
| const hasTriggerButton = !!activeGlobalDrawer?.trigger; | |||
| const animationDisabled = activeGlobalDrawer?.defaultActive && !drawersOpenQueue.includes(activeGlobalDrawer.id); | |||
| const isExpanded = activeGlobalDrawer?.isExpandable && expandedDrawerId === activeDrawerId; | |||
| const isExpandedPrevious = usePrevious(isExpanded); | |||
There was a problem hiding this comment.
💬 isExpandedPrevious can be called simpler - wasExpanded
| data-testid={`awsui-app-layout-drawer-content-${activeDrawerId}`} | ||
| > | ||
| {!isMobile && activeGlobalDrawer?.isExpandable && ( | ||
| <div className={styles['drawer-expanded-mode-button']}> |
There was a problem hiding this comment.
⛳ Creating grid layout column and manual position does not really scale.
Can we create a single "drawer buttons" wrapper that contains all current and future button additions?
| iconName={isExpanded ? 'shrink' : 'expand'} | ||
| onClick={() => setExpandedDrawerId(isExpanded ? undefined : activeDrawerId)} | ||
| variant="icon" | ||
| analyticsAction={isExpanded ? 'expand' : 'collapse'} |
There was a problem hiding this comment.
⛳ Did you check with @fralongo if this analytics action is already supported on the receivers' end?
There was a problem hiding this comment.
Yes. We don't need to do anything else on our end
| @@ -72,6 +73,17 @@ | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| &.drawer-expanded-mode { | |||
There was a problem hiding this comment.
⛳ "Skeleton layout" was supposed to be a very abstract implementation of the layout.
When expanded mode needs such heavy changes here, how do you expect it to widgetize and maintain in the future?
There was a problem hiding this comment.
how do you expect it to widgetize and maintain in the future?
Skeleton styles (this scss file) as well as its attributes (classNames based on component props) and app layout state will be widgetized, so that such changes will be delivered without merging from live.
| export default function WithDrawers() { | ||
| const { urlParams, setUrlParams } = useContext(AppContext as DemoContext); | ||
| const hasTools = urlParams.hasTools ?? true; | ||
| const appLayoutRef = useRef<AppLayoutProps.Ref>(null); |
There was a problem hiding this comment.
💬 This value is never used
| splitPanel={ | ||
| <SplitPanel | ||
| header="Overview" | ||
| i18nStrings={{ |
There was a problem hiding this comment.
💬 Do we really have no reusable strings for this already?
| test( | ||
| 'should show only expanded drawer and hide all other panels if expanded mode for a drawer is active', | ||
| setupTest(async page => { | ||
| await page.setWindowSize({ ...viewports.desktop, width: 1600 }); |
There was a problem hiding this comment.
💬 There are several places with this 1600 width already
Is this time to create a new viewports constant, something like "width to fit 3 side-by-side drawers"
| @@ -60,4 +60,6 @@ export interface AppLayoutInternals { | |||
| onActiveDrawerResize: (detail: { id: string; size: number }) => void; | |||
| onActiveGlobalDrawersChange: (newDrawerId: string, params: OnChangeParams) => void; | |||
| splitPanelAnimationDisabled?: boolean; | |||
| expandedDrawerId: string | undefined; | |||
There was a problem hiding this comment.
⛳ For widget compatibility, do we want to use null as "no drawer expanded" and then undefined as "we got an older version with this field not yet defined"
| }); | ||
| const { wrapper, globalDrawersWrapper } = await renderComponent(<AppLayout />); | ||
|
|
||
| await delay(); |
There was a problem hiding this comment.
💬 Why is there an extra delay? Was the delay inside await renderComponent() not enough?
# Conflicts: # package.json
… for global drawers
…edDrawerId: string | null
Description
Focus (expanded) mode for global drawers
q
kaB4Ap1KSFoxRelated links, issue #, if available: n/a
How has this been tested?
Unit / integ tests.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.